Skip to content

cephfs: move fscrypt lock into the CephFS RADOS namespace - #6539

Merged
mergify[bot] merged 5 commits into
ceph:develfrom
Greenpepper15:wip-cephfs-fscrypt-lock-namespace
Sep 11, 2026
Merged

mergify[bot] merged 5 commits into
ceph:develfrom
Greenpepper15:wip-cephfs-fscrypt-lock-namespace

Conversation

@Greenpepper15

@Greenpepper15 Greenpepper15 commented Sep 4, 2026

Copy link
Copy Markdown

Describe what this PR does

Fixes the first item of #6531: the RADOS lock that serializes fscrypt setup for encrypted volumes lives in the
default namespace of the metadata pool, while every other object Ceph-CSI creates for a CephFS cluster lives
in the configured RADOS namespace (cephFS.radosNamespace, default csi).

This PR places the lock in the RADOS namespace configured by cephFS.radosNamespace.

Is there anything that requires special attention

Is the change backward compatible?

Yes, I implemented a transitional legacy lock mechanism that keeps mixed-version deployments safe. This
mechanism makes nodes that include the changes of this PR try to get both the new lock (placed in
cephFS.radosNamespace) and the old lock (placed in the default RADOS namespace).

However, the "old lock" in the default RADOS namespace is not required in two cases. If the CephX capabilities
of the node cannot access the default namespace, the mount continues without it (such a deployment could never
stage an encrypted volume before this change, so no old nodeplugin can be holding the old lock). Volumes
without a RADOS namespace (static volumes) take only one lock, in the default namespace, because both locks
would be the same object. Any other failure to take the old lock fails the mount.

The legacy lock is marked with a TODO for removal once upgrades from releases that lock in the default
namespace are no longer supported.

Do you have any questions?

  • Is protecting the rolling-upgrade edge case worth the code it takes? The core fix is the single
    SetNamespace() call. The rest of the PR exists to keep an old nodeplugin and an upgraded one serialized
    while updating. The serialization only matters when the same encrypted volume is staged on a non-updated
    ceph-csi instance and an updated ceph-csi instance at the same time. If that window is considered
    unimportant, the legacy lock can be dropped (together with the sentinel-error commit it depends on and its
    e2e check). Instead the upgrade documentation would warn operators not to start pods that mount encrypted
    volumes while the nodeplugin upgrade is running. Ceph-CSI cannot enforce this since a nodeplugin does not
    know that a rolling upgrade is in progress, and the legacy lock is exactly the mechanism that serializes the
    two sides. Avoiding the race would be left entirely to the operator.

  • Is the e2e test's dependency on RADOS lock internals acceptable? The lock is released before
    NodeStageVolume returns, so the test can not observe it held. Instead it checks for the object that
    cls_lock leaves behind on unlock, which holds for every lock type except LOCK_EXCLUSIVE_EPHEMERAL. Should
    cls_lock ever stop leaving the object behind, or the fscrypt lock become ephemeral, the test breaks without
    any product regression. An e2e test was asked for in cephfs: cephFS.radosNamespace is not applied consistently #6531, so if this dependency is not acceptable, I would
    rework the check rather than drop it. However suggestions for a better way to observe the lock's namespace
    are welcome (I am unsure how to do it in a different manner).

Related issues

Part of #6531.

Future concerns

  • Remove the transitional legacy lock once upgrades from releases that take the fscrypt lock in the default
    namespace are no longer supported (marked with a TODO in the code).

AI assistance: this code was developed with the help of an AI assistant (Claude). Each commit carries the
Assisted-by: Claude Code <noreply@anthropic.com> trailer, per AGENTS.md.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

CI job ordering.

Depends-on: #6497

@Greenpepper15
Greenpepper15 requested review from a team as code owners September 4, 2026 10:30
@mergify mergify Bot added the component/cephfs Issues related to CephFS label Sep 4, 2026
nixpanic
nixpanic previously approved these changes Sep 8, 2026
@nixpanic
nixpanic requested a review from a team September 8, 2026 15:56
Madhu-1
Madhu-1 previously approved these changes Sep 9, 2026
Comment thread internal/cephfs/nodeserver.go
@nixpanic

nixpanic commented Sep 9, 2026

Copy link
Copy Markdown
Member

Wait for #6497 to be merged before starting full e2e and merging on this PR.

@mergify

mergify Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 ⛓️ Depends-On Requirements

Requirement based on the presence of Depends-On in the body of the pull request

🟢 🚦 Auto-queue

When all merge protections are satisfied, this pull request will be queued automatically.

@nixpanic

Copy link
Copy Markdown
Member

/queue

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

@Mergifyio rebase

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

rebase

❌ This pull request comes from a fork and cannot be rebased

Details

GitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that.

Use the update action or the @mergifyio update command instead: it brings the pull request up to date by merging the base branch into it, and needs no impersonation. It only has something to do when the pull request is behind its base branch, so if what the branch needs is a linear history, its author has to rebase it themselves.

@Greenpepper15

Greenpepper15 commented Sep 11, 2026

Copy link
Copy Markdown
Author

Should I rebase manually?
I don't want to confuse the mergify bot workflow so I will wait with rebasing until I get a confirmation.

@nixpanic

Copy link
Copy Markdown
Member

Should I rebase manually? I don't want to confuse the mergify bot workflow so I will wait with rebasing until I get a confirmation.

Hey @Greenpepper15 , yes, please rebase. The approvals will be dropped, but I'll get them added as soon as possible again. After the rebase, we'll start the full CI cycle.

In the mean time, we're looking into Mergify changes that allow rebasing/updating again.

Thanks!

LockExclusive folds every unexpected return value into one generic
error. Return a distinguishable error for EPERM and EACCES, so that
callers can tell a denied lock apart from a failed one. Locking is a
RADOS class operation, so it needs the class-exec permission on the
pool, and caps that cover the RADOS namespace of the IO context.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
The RADOS lock that serializes the fscrypt setup of an encrypted
volume should be placed in the RADOS namespace configured by
cephFS.radosNamespace and not in the default namespace of the
metadata pool, like every other object Ceph-CSI creates there.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Handle the rolling-upgrade window in which upgraded nodes take the
fscrypt lock in cephFS.radosNamespace while not-yet-upgraded nodes
still take it in the default namespace, unserialized against each
other.

Take both locks. The new lock is exclusively used when the old one is
not available. That is the case when the credentials have no
permission for the default namespace (such a deployment could never
stage an encrypted volume before this change, so no old node can hold
the old lock), and when the volume has no RADOS namespace (both locks
would be the same object).

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Mount an encrypted volume and check that the fscrypt lock object
exists in the CephFS RADOS namespace, and, while the transitional
legacy lock exists, in the default namespace as well. The lock is
released before NodeStageVolume returns; the check relies on cls_lock
leaving the object behind on unlock.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
@Greenpepper15
Greenpepper15 force-pushed the wip-cephfs-fscrypt-lock-namespace branch from cc135b2 to b087b71 Compare September 11, 2026 07:27
@mergify
mergify Bot dismissed stale reviews from Madhu-1 and nixpanic September 11, 2026 07:28

Pull request has been modified.

@nixpanic

Copy link
Copy Markdown
Member

/queue

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

@Mergifyio rebase

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

rebase

❌ This pull request comes from a fork and cannot be rebased

Details

GitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that.

Use the update action or the @mergifyio update command instead: it brings the pull request up to date by merging the base branch into it, and needs no impersonation. It only has something to do when the pull request is behind its base branch, so if what the branch needs is a linear history, its author has to rebase it themselves.

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.35

@ceph-csi-bot ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Sep 11, 2026
@mergify mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Sep 11, 2026
@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 11:50 UTC · Rule: default · triggered by merge protections
  • Checks failed · in-place
  • 🚫 Left the queue2026-09-11 12:29 UTC · at e4e3eb59c94fc2b4dd5a60fb97de18999ed65996

This pull request spent 39 minutes 42 seconds in the queue, with no time running CI.

Required conditions to merge

Reason

The merge conditions cannot be satisfied due to failing checks

  • Mergify Merge Protections
  • ci/centos/mini-e2e/k8s-1.34

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Requeued — the merge queue status continues in this comment ↓.

@ceph-csi-bot ceph-csi-bot added ok-to-test Label to trigger E2E tests and removed queued/rebase labels Sep 11, 2026
@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.35

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.35

@mergify mergify Bot added the queued label Sep 11, 2026
@ceph-csi-bot ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Sep 11, 2026
@mergify mergify Bot added dequeued and removed ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. queued labels Sep 11, 2026
@nixpanic

Copy link
Copy Markdown
Member

/retest ci/centos/mini-e2e/k8s-1.34

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 15:29 UTC · Rule: default · triggered by merge protections
  • Checks skipped · PR is already up-to-date
  • Merged2026-09-11 15:29 UTC · at 40ad6508c8d37241e8ea6819c8c63cae1f3020a7 · rebase

This pull request spent 18 seconds in the queue, including 3 seconds running CI.

Required conditions to merge

@mergify
mergify Bot merged commit 40ad650 into ceph:devel Sep 11, 2026
47 checks passed
@mergify mergify Bot removed the dequeued label Sep 11, 2026
@Greenpepper15

Greenpepper15 commented Sep 11, 2026

Copy link
Copy Markdown
Author

Nice thanks guys!
Onto the next one #6531

@Greenpepper15
Greenpepper15 deleted the wip-cephfs-fscrypt-lock-namespace branch September 14, 2026 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/cephfs Issues related to CephFS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants